auth required interception #960
Conversation
krichprollsch
force-pushed
the
auth-challenge
branch
2 times, most recently
from
96950fa to
fe3b9c2
Compare
krichprollsch
force-pushed
the
proxy-header
branch
from
8f57e83 to
7869159
Compare
krichprollsch
force-pushed
the
auth-challenge
branch
5 times, most recently
from
f8acde6 to
48b742b
Compare
krichprollsch
changed the title
sjorsdonkers
left a comment
sjorsdonkers
left a comment
There was a problem hiding this comment.
Code looks good.
I do think it is important to keep a flag in the intercept state whether it is normal or auth intercept, see comment.
I'll continue testing it now.
|
|
||
| var intercept_state = &bc.intercept_state; | ||
| const request_id = try idFromRequestId(params.requestId); | ||
| const transfer = intercept_state.remove(request_id) orelse return error.RequestNotFound; |
There was a problem hiding this comment.
I think we should add a flag to make sure that it was a auth interception and not a normal intercept.
Otherwise the situation may occur that a user sends a continueWithAuth as a responce to a requestPaused.
In that case for example if we call abortAuthChallenge we do not do the correct cleanup. In general we do not know what could happen, so better to catch it.
There was a problem hiding this comment.
What about testing if the transfer has a auth challenge?
Or you do want a complete separate intercept_state list?
There was a problem hiding this comment.
Checking and I guess then removing the auth challenge should work, then the opposite for continue/abort/fulfill
| // In this case we ignore callbacks for now. | ||
| // Note: we don't deinit transfer on purpose: we want to keep | ||
| // using it for the following request. | ||
| self.endTransfer(transfer); |
There was a problem hiding this comment.
I wonder whether we need to send a transfer.resetCallback as consumer may need to reset their state for the replaced intercept to be sent.
Or we can make it part of the contract that this is the case when transfer.start_callback is called for the same transfer, but we probably should document that perhaps near: start_callback: ?*const fn (transfer: *Transfer) anyerror!void = null, in Request.
| don't see issue with the current consumers implementation.
There was a problem hiding this comment.
Hum, I suppose the point of the interception is to take place during the request lifecycle. So the following callbacks will be called depending the result of the interception and the optional request following.
krichprollsch
force-pushed
the
auth-challenge
branch
2 times, most recently
from
0facf97 to
9d4bc72
Compare
krichprollsch
force-pushed
the
auth-challenge
branch
from
9d4bc72 to
5defb5c
Compare
No description provided.